-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed bugs in Spec parser for terminals (#145) #146
Conversation
@@ -88,7 +88,7 @@ trait Parsers extends LangParsers { | |||
|
|||
/** terminals */ | |||
lazy val term: Parser[Terminal] = { | |||
"`[^`]+`|```".r ^^ { | |||
"`[^`]+`|`[`]+`".r ^^ { | |||
case str => | |||
Terminal(str.substring(1, str.length - 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't 1
be replaced with the length of the delimiter here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would the length of the delimiter not be 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure what 4 or more backticks in a row is supposed to mean. It doesn't appear to be legal. So I don't know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ecmarkup accepts 4 backticks and renders it as a literal terminal consisting of two backticks: 2418 preview
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reading the code for grammarkdown and it doesn't seem it would support 4 backticks in a row, though I could be misreading. Does it have something to do with tc39/ecma262#2418 using `
and the entities being processed by esmeta but not grammarkdown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESMeta unescapes the HTML entities before parsing the spec.html
. Thus, the text ````
is unescaped as: ````
, and ESMeta parses the four backticks (````
) as a literal terminal consists of two backticks (``
) because the first and last backtick is used to represent literal terminals. I supported this feature because tc39/ecma262#2418 defines the literal terminal consisting of two backticks. Is there any problem with this extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that should be fine then. I didn't know about the pre-processing of HTML entities by esmeta.
@@ -88,7 +88,7 @@ trait Parsers extends LangParsers { | |||
|
|||
/** terminals */ | |||
lazy val term: Parser[Terminal] = { | |||
"`[^`]+`|```".r ^^ { | |||
"`[^`]+`|`[`]+`".r ^^ { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs for grammarkdown don't mention this: https://github.com/rbuckton/grammarkdown#terminals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few things (used by the ES spec) that the grammarkdown docs don't mention or aren't clear on. I think that's more of an issue for grammarkdown than for esmeta.
I fixed a bug in the
Spec
parser for terminals.The PR tc39/ecma262#2418 in ecma262 defines a new terminal:
``
as follows:However, the current
Spec
parser for terminals only supports a single quote`
. So, I fixed this problem by extending theSpec
parser to support one or more repetitions of`
.